fix(baseball): #379 Phase 4b engine reader migration + (player,date)-scoped precedence (supersedes #852) - #854
Conversation
…weep/baseline/engine-run + deterministic import-quality clock (#379, #811 residual) Reader migration (#379 Phase 4b, the highest-blast-radius chunk): - NEW src/lib/baseball/coachhelm/engine-stat-rows.ts — the ONE consolidated per-session stat-row read for the engine. Prefers canonical baseball_box_score_batting/_pitching rows (normalized onto the loader shape via #851's normalizeBoxScoreBattingRow/normalizeBoxScorePitchingRow, with session_date joined from baseball_games and source-table provenance tags), reconciled over legacy baseball_player_stats rows per the #379 precedence rule: canonical rows replace a player's legacy GAME rows outright (never blended — the #827 seed writes the same games into both layers), legacy practice/other rows always survive (practice carve-out), and a player with zero canonical rows keeps full legacy history (fallback tier). Canonical-side read failures degrade all-or-nothing to the legacy pool; a legacy read failure remains the callers' hard error. All reads paginate past the PostgREST 1000-row cap with stable ordering. - outcome-sweep.ts / action-baseline.ts / engine-run.ts all swap their direct baseball_player_stats reads for the shared helper, so baseline capture, the outcome sweep, and the engine run measure the SAME reconciled pool (apples-to-apples did-it-move). action-baseline's old single-page .limit(1000) read is replaced by the paginated shared read. #811 residual (deterministic engine clock): - BaseballV10EngineInputs gains an optional now (ISO); engine-run threads its nowIso through it; importQualityGenerator's 14-day recency window computes from the caller-supplied nowIso instead of raw Date.now() (default preserves real-time behavior for non-engine callers). New generators/v10.test.ts pins the window against a fixed 2020 clock; engine-run-helm-lifting.test.ts pins that runBaseballEngineCore passes its own nowIso end-to-end. Provenance labels: - generators/index.ts driver() last-resort fallback label no longer hardcodes the deprecated table (loaders now cite the real per-row table); v10.ts's practice-effectiveness cite stays deliberately (its feeder still reads legacy practice rows) with an explanatory comment. Manifest (stat-layer contract kept green in both directions for this chunk): - Removed migrated entries: outcome-sweep.ts, engine-run.ts, action-baseline.ts, generators/index.ts, ai-policy-enforcement.test.ts, signal-from-insight.test.ts (fixtures moved to canonical table names). - Added: engine-stat-rows.ts + its test (the one allowed legacy-fallback read). - Updated notes: loaders.ts, generators/v10.ts, effectiveness/engine.ts and operational-rule-engine.ts (both reviewed, deliberately deferred — their cites are honest while their feeders still read layer 1), plus the three engine test entries now pinning the fallback tier. Tests: engine-stat-rows.test.ts pins the precedence rule directly; action-baseline.test.ts + outcome-sweep-insight-resolve.test.ts gain canonical-preferred, never-blended coverage alongside the existing legacy-fallback pins. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa
…ot player alone (#379, #852 review fix) The #379 exclusion rule dropped ALL of a player's legacy stat_type='game' rows once they had ANY canonical box-score row, even for games with no canonical counterpart — a mid-season box-score import start would silently erase that player's earlier legacy-logged games from every engine caller (engine-run, outcome-sweep, action-baseline), shrinking sample_n and flipping confidence/verdicts. baseball_player_stats has no game_id, so we now correlate on the resolved canonical game date (truncated to YYYY-MM-DD on both sides) instead: a legacy game row is dropped only when that same player has canonical coverage on that exact calendar day: a same-day heuristic, not a guaranteed game-identity match, since there's no FK to lean on (documented in the module comment as an accepted double-header collision risk). Existing precedence tests encoded the bug: their legacy-row fixture dates never matched the canonical game dates, yet still asserted full drop — only possible under the old player-scoped exclusion. Realigned those fixture dates to same-day overlap (preserving each test's 100%-coverage intent) and added a mixed-coverage case (3 legacy-only + 2 canonical -> 5 rows survive) plus a same-day-collision case (legacy row on a canonically-covered date -> dropped). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (3)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Greptile SummaryThis PR lands Phase 4b of #379: a single
Confidence Score: 4/5Safe to merge. The new consolidated read correctly applies the (player, date)-scoped precedence rule, degrades cleanly to legacy on canonical-side errors, and the three migrated callers are wired identically — keeping baseline/observed apples-to-apples. The implementation is logically sound: the precedence Set is built from already player-filtered canonical rows, the ALL-OR-NOTHING degrade prevents partial blends, and the nowIso threading closes the last rolling-window clock leak. The only gap is the primary unit test file's no-op .eq()/.in() fake, which means team-scoping of canonical queries is not exercised by tests — a regression there would require a production incident to detect. src/lib/baseball/tests/engine-stat-rows.test.ts — the fake client skips .eq()/.in() filtering; consider adding a cross-team isolation assertion before this module graduates out of the grandfathered list. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
ER[engine-run.ts] --> LSR
OS[outcome-sweep.ts] --> LSR
AB[action-baseline.ts] --> LSR
LSR["loadEngineStatRows(db, teamId, playerIds)"]
LSR --> P1[baseball_player_stats]
LSR --> P2[baseball_games]
LSR --> P3[baseball_box_score_batting]
LSR --> P4[baseball_box_score_pitching]
P1 --> M{Promise.all}
P2 --> M
P3 --> M
P4 --> M
M -->|legacy read error| HE[return null + error]
M -->|any canonical error| DEG[degrade to legacyRows only]
M -->|zero canonical rows| LEG[legacy fallback unchanged]
M -->|canonical rows present| NORM[normalizeBoxScore rows]
NORM --> SET[Build canonicalPlayerDates Set]
SET --> FILTER[Drop legacy game rows on covered dates]
FILTER --> OUT[return normalized + retainedLegacy]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
ER[engine-run.ts] --> LSR
OS[outcome-sweep.ts] --> LSR
AB[action-baseline.ts] --> LSR
LSR["loadEngineStatRows(db, teamId, playerIds)"]
LSR --> P1[baseball_player_stats]
LSR --> P2[baseball_games]
LSR --> P3[baseball_box_score_batting]
LSR --> P4[baseball_box_score_pitching]
P1 --> M{Promise.all}
P2 --> M
P3 --> M
P4 --> M
M -->|legacy read error| HE[return null + error]
M -->|any canonical error| DEG[degrade to legacyRows only]
M -->|zero canonical rows| LEG[legacy fallback unchanged]
M -->|canonical rows present| NORM[normalizeBoxScore rows]
NORM --> SET[Build canonicalPlayerDates Set]
SET --> FILTER[Drop legacy game rows on covered dates]
FILTER --> OUT[return normalized + retainedLegacy]
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
src/lib/baseball/__tests__/engine-stat-rows.test.ts:182-201
**Test fake silently skips team-scoping on canonical reads**
The fake client's `.eq()` and `.in()` are no-ops (`eq: () => builder`, `in: () => builder`), so no test in this file verifies that the canonical queries carry `.eq('team_id', teamId)`. If that filter were accidentally removed from `engine-stat-rows.ts`, every test here would still pass while the production read would cross team boundaries.
The `action-baseline.test.ts` fake *does* filter on `eq`, and the schema + RLS enforce isolation in production — so there's no current data leakage — but the gap means a future refactor of the canonical read chain could silently drop the `team_id` predicate without a test failure. Adding a per-table row-count assertion after seeding rows for a second team (with a different `team_id`) would close the gap without restructuring the rest of the test suite.
Reviews (1): Last reviewed commit: "fix(baseball): scope engine-stat-rows pr..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| legacyGame('lg-1', 'p1', { session_date: '2026-01-01' }), | ||
| legacyGame('lg-2', 'p1', { session_date: '2026-01-08' }), | ||
| legacyGame('lg-3', 'p1', { session_date: '2026-01-15' }), | ||
| ], | ||
| baseball_games: [ | ||
| { id: 'g1', game_date: '2026-04-01' }, | ||
| { id: 'g2', game_date: '2026-04-08' }, | ||
| ], | ||
| baseball_box_score_batting: [ | ||
| boxBatting('bb-1', 'p1', 'g1'), | ||
| boxBatting('bb-2', 'p1', 'g2'), | ||
| ], | ||
| }); | ||
|
|
||
| const { data, error } = await loadEngineStatRows(client, TEAM, ['p1']); | ||
| expect(error).toBeNull(); | ||
| // 3 legacy-only + 2 canonical = 5 rows; nothing here collides on date, so | ||
| // the player-scoped bug (dropping ALL legacy games once ANY canonical row | ||
| // exists) would have wrongly shrunk this to 2. | ||
| const ids = data!.map((r) => r.id).sort(); |
There was a problem hiding this comment.
The fake client's .eq() and .in() are no-ops (eq: () => builder, in: () => builder), so no test in this file verifies that the canonical queries carry .eq('team_id', teamId). If that filter were accidentally removed from engine-stat-rows.ts, every test here would still pass while the production read would cross team boundaries.
The action-baseline.test.ts fake does filter on eq, and the schema + RLS enforce isolation in production — so there's no current data leakage — but the gap means a future refactor of the canonical read chain could silently drop the team_id predicate without a test failure. Adding a per-table row-count assertion after seeding rows for a second team (with a different team_id) would close the gap without restructuring the rest of the test suite.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/baseball/__tests__/engine-stat-rows.test.ts
Line: 182-201
Comment:
**Test fake silently skips team-scoping on canonical reads**
The fake client's `.eq()` and `.in()` are no-ops (`eq: () => builder`, `in: () => builder`), so no test in this file verifies that the canonical queries carry `.eq('team_id', teamId)`. If that filter were accidentally removed from `engine-stat-rows.ts`, every test here would still pass while the production read would cross team boundaries.
The `action-baseline.test.ts` fake *does* filter on `eq`, and the schema + RLS enforce isolation in production — so there's no current data leakage — but the gap means a future refactor of the canonical read chain could silently drop the `team_id` predicate without a test failure. Adding a per-table row-count assertion after seeding rows for a second team (with a different `team_id`) would close the gap without restructuring the rest of the test suite.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Supersedes #852 (same chunk rebased onto the current batch tip + the review's critical fix; fresh branch to avoid a force-push).
What this is
The full #379 Phase 4b chunk (engine-stat-rows.ts consolidated read; outcome-sweep/action-baseline/engine-run migrated; #811 clock residual closed) PLUS the adversarial-review fix: legacy game-row exclusion is now scoped to (player_id, session_date) against canonical game dates — a transitional player's un-box-scored history survives (was: any single canonical row dropped ALL the player's legacy games). Same-day heuristic limitation documented in-code (legacy table has no game_id).
Review trail
Gates
typecheck exit 0 · eslint --max-warnings 0 clean (all touched) · targeted engine suites 184/184 · full test:business 5193/5217 passed, 10 failures = the pre-existing stat-layer-contract (owned by #845/#847) + FairwayRecruitingPage ResizeObserver set, confirmed absent from this diff (git diff --stat vs base).
🤖 Generated with Claude Code
https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa